Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bugfix/201 fix some typo and correct expected return types #202

Merged
merged 8 commits into from
Jan 9, 2025
Merged

Bugfix/201 fix some typo and correct expected return types #202

merged 8 commits into from
Jan 9, 2025

Conversation

MustafaJafar
Copy link
Contributor

Changelog Description

resolve #201

Note:
get_invalid should return None or list of hou.Node/ hou.OpNode

Testing notes:

  1. Error reported in the linked issue should be fixed
  2. Everything should work as before

@MustafaJafar MustafaJafar added the type: bug Something isn't working label Dec 23, 2024
@MustafaJafar MustafaJafar self-assigned this Dec 23, 2024
moonyuet
moonyuet requested changes Dec 24, 2024
View reviewed changes
Copy link
Member

@moonyuet moonyuet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValidateSingleFrame has been errored out
image
Uploading publish-report-241224-17-57.json…

moonyuet
moonyuet approved these changes Dec 27, 2024
View reviewed changes
Copy link
Member

@moonyuet moonyuet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now!
image

@BigRoy
Copy link
Contributor

BigRoy commented Dec 27, 2024

I wonder if it makes sense to let our IDEs help us out a hand in the future - and add Type Hints to the get_invalid methods so it's hinted to be Optional[List["hou.Node"]]?

@MustafaJafar
Copy link
Contributor Author

I wonder if it makes sense to let our IDEs help us out a hand in the future - and add Type Hints to the get_invalid methods so it's hinted to be Optional[List["hou.Node"]]?

This will be cool!
Note:
It's expected that get_invalid returns Optional[List["hou.Node"]] only when we are using select invalid action.

BigRoy
BigRoy reviewed Dec 27, 2024
View reviewed changes
Copy link
Contributor

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the change looks good - but in most cases you're not refactoring how we're formatting the node into the PublishValidationError which means the message there will turn out worse. So we may want to make sure there that we're still formatting using the path of the nodes.

@BigRoy
Copy link
Contributor

BigRoy commented Dec 27, 2024

Note: It's expected that get_invalid returns Optional[List["hou.Node"]] only when we are using select invalid action.

Yup, am aware - but I have a feeling that should be 99% of the use cases of the get_invalid class methods or static methods. (And I'd rather have it for all of them to avoid confusion if suddenly one does not behave that way.) And if there's indeed a reason for one of them to deviate, hopefully seeing the type hints differ there will also help make sense of things.

@MustafaJafar
Copy link
Contributor Author

MustafaJafar commented Jan 7, 2025 •
edited
Loading

Note: It's expected that get_invalid returns Optional[List["hou.Node"]] only when we are using select invalid action.

Yup, am aware - but I have a feeling that should be 99% of the use cases of the get_invalid class methods or static methods. (And I'd rather have it for all of them to avoid confusion if suddenly one does not behave that way.) And if there's indeed a reason for one of them to deviate, hopefully seeing the type hints differ there will also help make sense of things.

what about implementing get_invalid on HoudiniInstancePlugin and HoudiniContextPlugin.
most of the time,
get_invalid on context plugins is most of the times: get_invalid(cls, context)
get_invalid on instance plugins is most of the times: get_invalid(cls, instance)

@MustafaJafar MustafaJafar requested a review from BigRoy January 7, 2025 12:12
moonyuet
moonyuet approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@moonyuet moonyuet left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with publishing the model product types in Houdini. Validators looks good for me.
image

@moonyuet
Copy link
Member

moonyuet commented Jan 9, 2025 •
edited
Loading

One thing is a worth mention that somehow validate resolution setting does not validate the resolution correctly in my case. Even if I set the resolution back to 1920X1080, the validator still blocks me from publishing the render (Do it in separate PR as it's not related to this PR)
image

@MustafaJafar
Copy link
Contributor Author

One thing is a worth mention that somehow validate resolution setting does not validate the resolution correctly in my case. Even if I set the resolution back to 1920X1080, the validator still blocks me from publishing the render (Do it in separate PR as it's not related to this PR) image

This is weird, I was not able to replicate it on my side.
could you create an issue with reproducible steps ?

@MustafaJafar MustafaJafar merged commit 9a6d47e into develop Jan 9, 2025
1 check passed
@MustafaJafar MustafaJafar deleted the bugfix/201-fix-some-typo-and-correct-expected-return-types branch January 9, 2025 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moonyuet moonyuet moonyuet approved these changes

@BigRoy BigRoy Awaiting requested review from BigRoy

Assignees

@MustafaJafar MustafaJafar

Labels
type: bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix some typo and correct expected return types
3 participants
@MustafaJafar @BigRoy @moonyuet